tests/test_sdk_shell_session.py (1)

315-323: Consider explicitly managing the background shutdown task.

The fire-and-forget task pattern here works, but it's implicit. While Python 3.10+ guarantees the task won't be collected, explicitly retaining a reference and cleaning it up in a finally block (or using a context manager) would make the intent clearer and reduce the risk of subtle issues if the code evolves.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_sdk_shell_session.py` around lines 315 - 323, The test starts a
fire-and-forget task via asyncio.create_task(_set_shutdown()) without keeping a
reference; change the _run coroutine to store the task (e.g., shutdown_task =
asyncio.create_task(_set_shutdown())), then wrap the call to forward_io(ws, [80,
24], shutdown) in a try/finally where in finally you cancel the shutdown_task
(if still running) and await it to ensure cleanup; reference symbols to edit:
_run, _set_shutdown, shutdown, forward_io, and the asyncio.create_task call.

centml/cli/shell.py (2)

41-60: ⚠️ Potential issue | 🟠 Major

Guard against prompting in non-interactive contexts.

exec_cmd can be invoked non-interactively (CI, scripts), but _connect_args may call _select_pod which uses click.prompt(). This will hang or fail when multiple pods exist and --pod/--first-pod is not specified. Consider requiring --pod or --first-pod when stdin is not a TTY, or defaulting to first pod in non-interactive mode.

🛠️ Proposed fix

 def _connect_args(deployment_id, pod, shell_type, first_pod=False):
     """Resolve pod, build WebSocket URL, and obtain auth token."""
     with get_centml_client() as cclient:
         running_pods = get_running_pods(cclient, deployment_id)
         if not running_pods:
             raise click.ClickException(f"No running pods found for deployment {deployment_id}")
 
         if pod is not None:
             try:
                 pod_name = _resolve_pod(running_pods, pod)
             except ShellError as exc:
                 raise click.ClickException(str(exc)) from exc
         elif len(running_pods) == 1 or first_pod:
             pod_name = running_pods[0]
+        elif not sys.stdin.isatty():
+            raise click.ClickException(
+                "Multiple pods found. Specify --pod or --first-pod for non-interactive use."
+            )
         else:
             pod_name = _select_pod(running_pods, deployment_id)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@centml/cli/shell.py` around lines 41 - 60, The _connect_args function can
block by calling _select_pod (which uses click.prompt) in non-interactive runs;
fix by detecting interactivity (e.g. using sys.stdin.isatty()) and if not
interactive and pod is None and not first_pod and len(running_pods) > 1, do not
call _select_pod: instead raise a click.ClickException telling the user to
provide --pod or --first-pod (or alternatively choose to default to
running_pods[0] if you prefer non-interactive default behavior); implement this
check inside _connect_args before calling _select_pod so build_ws_url and
auth.get_centml_token remain unchanged.

---

80-92: ⚠️ Potential issue | 🟠 Major

Use shlex.join() to preserve command argument boundaries.

" ".join(command) loses quoting and escaping. Arguments like "hello world", $HOME, or * will be misinterpreted by the pod shell. Use shlex.join(command) for correct POSIX shell escaping.

🛠️ Proposed fix

+import shlex
 ...
 def exec_cmd(deployment_id, command, pod, shell_type, first_pod):
     ws_url, token = _connect_args(deployment_id, pod, shell_type, first_pod)
-    exit_code = asyncio.run(exec_session(ws_url, token, " ".join(command)))
+    exit_code = asyncio.run(exec_session(ws_url, token, shlex.join(command)))
     sys.exit(exit_code)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@centml/cli/shell.py` around lines 80 - 92, The pod exec command concatenates
arguments with " ".join(command) which loses quoting/escaping; change exec_cmd
to construct the shell string with shlex.join(command) before calling
exec_session so argument boundaries are preserved (update the call in exec_cmd
that currently does asyncio.run(exec_session(ws_url, token, " ".join(command)))
to use shlex.join(command)); ensure you import shlex at top of
centml/cli/shell.py if not already present.

centml/sdk/shell/__init__.py (1)

4-12: Consider sorting __all__ for consistency.

Ruff (RUF022) flags that __all__ is not sorted. Sorting improves maintainability and aligns with isort-style conventions.

♻️ Proposed fix

 __all__ = [
-    "ShellError",
     "NoPodAvailableError",
      "PodNotFoundError",
+    "ShellError",
     "build_ws_url",
+    "exec_session",
     "get_running_pods",
     "interactive_session",
-    "exec_session",
 ]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@centml/sdk/shell/__init__.py` around lines 4 - 12, The __all__ list is
unsorted (RUF022); sort the exported names alphabetically to satisfy Ruff and
maintain consistency — reorder the entries in __all__ so identifiers like
"NoPodAvailableError", "PodNotFoundError", "ShellError", "build_ws_url",
"exec_session", "get_running_pods", "interactive_session" appear in ascending
lexicographic order (update the __all__ variable in centml/sdk/shell/__init__.py
where it's defined).

tests/test_sdk_shell_session.py (1)

315-323: Store the task reference to prevent potential garbage collection.

The asyncio.create_task() return value is not stored, which could theoretically allow the task to be garbage collected before completion. While this test likely works in practice, storing the reference is the recommended pattern.

♻️ Suggested fix

                 async def _run():
                     async def _set_shutdown():
                         await asyncio.sleep(0.1)
                         shutdown.set()

-                    asyncio.create_task(_set_shutdown())
+                    _ = asyncio.create_task(_set_shutdown())
                     return await forward_io(ws, [80, 24], shutdown)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_sdk_shell_session.py` around lines 315 - 323, The create_task call
in the local coroutine _run uses asyncio.create_task(_set_shutdown()) without
storing its return value, which risks the task being garbage-collected; assign
the created Task to a local variable (e.g., shutdown_task or setter_task) inside
_run so the reference lives at least until forward_io(ws, [80, 24], shutdown)
completes (optionally await or let it finish naturally), keeping function names
_run and _set_shutdown unchanged.

centml/sdk/shell/session.py (2)

127-136: Consider narrowing the exception catch during task cleanup.

The except (asyncio.CancelledError, Exception) catch is broad and silently suppresses all exceptions from cancelled tasks. While this is common in cleanup code, it could mask unexpected errors. Consider catching specific expected exceptions or at minimum logging unexpected ones.

♻️ Suggested improvement

     for t in pending:
         try:
             await t
-        except (asyncio.CancelledError, Exception):
-            pass
+        except asyncio.CancelledError:
+            pass  # Expected for cancelled tasks
+        except websockets.ConnectionClosed:
+            pass  # Expected during shutdown
+        except Exception:
+            pass  # Suppress other errors during cleanup

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@centml/sdk/shell/session.py` around lines 127 - 136, The cleanup loop
currently swallows all exceptions with "except (asyncio.CancelledError,
Exception): pass", which can hide real errors; update the loop that awaits tasks
in "pending" to handle asyncio.CancelledError separately and to catch Exception
as "e" and log it (e.g. using the module/session logger) instead of silencing
it, so reference the variables pending, done, and t and replace the broad except
with a specific asyncio.CancelledError branch and an Exception branch that logs
the unexpected exception details.

---

164-168: Unhandled exceptions from fire-and-forget coroutine.

asyncio.ensure_future(ws.send(...)) schedules the send but doesn't handle potential exceptions. If the WebSocket is closed when a resize signal arrives, this could produce unhandled exception warnings.

♻️ Suggested fix with exception handling

             def _send_resize():
                 c, r = shutil.get_terminal_size()
                 term_size[0], term_size[1] = c, r
-                asyncio.ensure_future(ws.send(json.dumps({"operation": "resize", "rows": r, "cols": c})))
+                async def _do_resize():
+                    try:
+                        await ws.send(json.dumps({"operation": "resize", "rows": r, "cols": c}))
+                    except websockets.ConnectionClosed:
+                        pass  # Connection already closed, ignore resize
+                asyncio.ensure_future(_do_resize())

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@centml/sdk/shell/session.py` around lines 164 - 168, The _send_resize closure
currently calls asyncio.ensure_future(ws.send(...)) which can raise unhandled
exceptions if the websocket is closed; change this to schedule a coroutine that
catches and logs or ignores exceptions from ws.send. Create an async helper
(e.g., async def _safe_send_resize(rows, cols): try: await ws.send(...); except
Exception as e: process or debug-log the error) and call
asyncio.create_task(_safe_send_resize(r, c)) inside _send_resize so resize
failures are handled gracefully; reference _send_resize, ws.send, term_size, and
the existing asyncio scheduling call when making the change.